Skip to content

Add attributes free-form map field for tool-specific fields in a number of places #214

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 21 commits into from
Nov 23, 2020

Conversation

davidfestal
Copy link
Collaborator

@davidfestal davidfestal commented Nov 10, 2020

What does this PR do?

This PR adds the attributes map, to enable adding tool-specific fields, at the root of the following objects:
components, projects, starterProjects and commands.

The attributes map is not only a map of strings, but also allows adding any json-schema values in map values (YAML values in yaml documents or Json values in JSON documents). here is an example:

components:
  - name: a-container
    attributes:
      tool1-string-attribute: a-string
      tool2-object-attribute:
        free-form-string-field: "value" 
        free-form-boolean-field: true 
      tool3-array-attribute:
        - value1 
        - value2 

This PR also updates the attributes already present in the container endpoints definition to allow objects in map values. This is not a breaking change though since the current proposal also covers the "map of strings" case.

In go the attribute values would be retrieved through one of the Attributes methods, some of which are demonstrated in the following snippet:

component : ...
attributes := component.Attributes

var err error

withoutErrorChecking := attributes.GetString("tool1-string-attribute")

withErrorChecking := attributes.GetString("tool1-string-attribute", &err)

myStruct := struct {
  StringField string `json:"free-form-string-field"`
  BooleanField string `json:"free-form-boolean-field"`
}

err = attributes.GetInto("tool2-object-attribute", &myStruct)

var arrayValues []interface{} = attributes.Get("tool3-array-attribute")

What issues does this PR fix or reference?

This fixes issue #169

Is your PR tested? Consider putting some instruction how to test your changes

Yes, it is.

Signed-off-by: David Festal <[email protected]>
Signed-off-by: David Festal <[email protected]>
@davidfestal davidfestal requested review from elsony and l0rd November 10, 2020 13:00

// Attributes provides a way to add a map of arbitrary YAML/JSON
// objects.
type Attributes map[string]apiext.JSON
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from the model point of view, it's so useful to have any JSON in the field, but from client(odo, devworkspace) perspective of view, it means that schema does not validate attributes format and every field access requires checking that attributes have the right format, if I did not miss something. So, we need some library code that allows easily to get attribute value as string, int, interface or error if the field has the wrong format.
Am I missing something and it's not going to be an issue for clients to read the attributes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, I'm looking into it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To add here, I don't know how elegant or useful embedding full json as an attribute will ultimately be. Even with map[string]string you could ostensibly use json, but the main problem is that it's hard to write. I think I'd rather see something like

attributes:
  controller.devfile.io/plugin/name: "vscode-java"
  controller.devfile.io/plugin/vscode-extensions/java: "https://my-url.com/java.vsix"
  controller.devfile.io/plugin/vscode-extensions/java-debug: "https://my-url.com/java-debug.vsix"

than have the above collapsed into a json blob

attributes:
  controller.devfile.io/plugin: '{"name": "vscode-java", "plugins": ["java.vsix", "java-debug.vsix"]}'

The latter could get quite complicated as there are longer entries. It also forces attributes to be complex, as we couldn't use something like my-attribute: my-value.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

than have the above collapsed into a json blob

@amisevsk we just don't need to have json collasped into a JSON blob.
This is a json-like arbitrary value, directly included inside the JSON or YAML document.
So in a YAML document it will a yaml fragment, and in a JSON document it will be a JSON document.

Your example would be:

    attributes:
      name: vscode-java
      plugins:
        - java.vsix
        - java-debug.vsix

That's the whole point of the change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And with the helpers I'm currently adding to the PR, when getting the GO structs, you would extract it like that:

var Attributes attributes = ...
myName, isAString := attributes.Get("name").Interface().(string)

// And for plugins you might use want to use the `DecodeInto` method, to decode the arbitrary JSON-like structure into a given, well-known GO type. 

plugins := &[]string{}
err := attributes.Get("plugins").DecodeInto(plugins)

// Of course you also have a method to know if a given key is available in the Attributes:

isThere := attributes.Exists("aMissingKey")

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense to me and is a lot cleaner than my suggestion :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering if it makes sense to have these helper functions in devfile/api or devfile/library

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maysunfaisal In fact it is there, because my plan was to also use it in the DevfileMetadata struct, which is also in the devfile/api repo, but in the non-K8S-versioned devfile package.

Signed-off-by: David Festal <[email protected]>
Copy link
Contributor

@amisevsk amisevsk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no suggestions apart from the point brought up by Serhii

@davidfestal davidfestal changed the title Add attributes free-form map field for tool-specific fields in a number of places [WIP] Add attributes free-form map field for tool-specific fields in a number of places Nov 12, 2020
Signed-off-by: David Festal <[email protected]>
@davidfestal davidfestal changed the title [WIP] Add attributes free-form map field for tool-specific fields in a number of places Add attributes free-form map field for tool-specific fields in a number of places Nov 16, 2020
@davidfestal
Copy link
Collaborator Author

I simplified the attributes api helpers and added many tests.

@amisevsk @sleshchenko on this personnal branch amisevsk/devworkspace-operator@use-v1alpha2...davidfestal:use-v1alpha2-and-new-attributes you will find examples of use in the changes I made on @amisevsk use-v1alpha2 to accomodate the new attributes in the endpoints case.

Still need to document public methods in the new attributes package though.

Signed-off-by: David Festal <[email protected]>
Copy link
Contributor

@amisevsk amisevsk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM

if attribute, exists := attributes[key]; exists {
container := &[]interface{}{}
err := json.Unmarshal([]byte("[ "+string(attribute.Raw)+" ]"), container)
if err != nil && len(errorHolder) > 0 && errorHolder != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should return the error instead of passing in a struct optionally. This approach makes it very easy to hide ignoring a potential error.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The whole point of this approach is to avoid requiring a multiple value return, in order to be able to chain calls when we know there is no reason for an error.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But here I can change it. I'd be more reluctant to change it for the GetString()... methods, since it's quite useful to be able to chain them or directly use the result in an expression.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me, just wanted to make sure there was a reason for doing it this way.

Copy link
Member

@sleshchenko sleshchenko Nov 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when we know there is no reason for an error

Can we identify when we know when there is a reason for the error and when there is no?
Like in the shared example amisevsk/devworkspace-operator@use-v1alpha2...davidfestal:use-v1alpha2-and-new-attributes#diff-cee3fd6d4114264b1f235c57bd27c71a9c40cd809efa214a395f5e4cfee8a92fR80
is it safe to propagate an empty string in case the user misconfigured protocol or path as an object?
It shows that such an aproach forces client not to check the erorr. As an alternative, we could have two separate methods(for each type) one would return the error, another will ignore the error but would be named in the way client understand error is suppressed. Like GetStringOrDefault(), default depends on type: string - "", int - 0, interface - {}. <- not the best name since IMHO makes the impression that default is expected as an argument, which also could be a solution for us.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More relevant case, where some boolean value is mistyped by user, like public: "true", instead of public: true. Then user will get unexpected behavior instead of error:

func TestBooleanAsString(t *testing.T) {
	stringMap := Attributes{}.FromStringMap(map[string]string{
		"public": "true",
	})
	publicAttr := stringMap.GetBoolean("public")
	assert.Equal(t, true, publicAttr)
}

@@ -22,4 +26,6 @@ type DevfileMetadata struct {
// +optional
// +kubebuilder:validation:Pattern=^([0-9]+)\.([0-9]+)\.([0-9]+)(\-[0-9a-z-]+(\.[0-9a-z-]+)*)?(\+[0-9A-Za-z-]+(\.[0-9A-Za-z-]+)*)?$
Version string `json:"version,omitempty"`

attributes.Attributes `json:",inline"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be json:"attributes,omitempty"

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was trying to have the free-form attributes at the same level as the main metadata, but this is obviously not really possible.
I finally changed it according to your suggestion in my last commit: 1a8f497

Signed-off-by: David Festal <[email protected]>
Copy link
Contributor

@amisevsk amisevsk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after updates.

@maysunfaisal
Copy link
Member

@davidfestal looks good 👍

Copy link
Contributor

@yangcao77 yangcao77 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks David for this PR, good changes! lgtm

Copy link
Contributor

@amisevsk amisevsk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of questions but otherwise LGTM.

Signed-off-by: David Festal <[email protected]>
... when decoding into a value from a `nil` `Attributes` value

Signed-off-by: David Festal <[email protected]>
Signed-off-by: David Festal <[email protected]>
@davidfestal
Copy link
Collaborator Author

davidfestal commented Nov 19, 2020

As discussed today, we're waiting for you GO to merge this @elsony, based of the devfile parser library readiness.

@elsony
Copy link
Contributor

elsony commented Nov 19, 2020

@davidfestal I have checked with the team. It turns out that the parser code is currently pulling in a specific tag of the schema so merging this change is not going to break the parser code. I also checked with @mmulholl and he'll adjust his test PR to accommodate that. Given that his test code has not been merged yet, so it is not going to be a problem. So feel free to merge this PR whenever you think is ready.

... as explained in the following comment:
#214 (comment)

Signed-off-by: David Festal <[email protected]>
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: amisevsk, davidfestal, l0rd, maysunfaisal, sleshchenko, yangcao77
To complete the pull request process, please assign after the PR has been reviewed.
You can assign the PR to them by writing /assign in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@davidfestal davidfestal merged commit 3793734 into master Nov 23, 2020
@davidfestal davidfestal deleted the attributes branch November 23, 2020 15:08
amisevsk pushed a commit to amisevsk/devworkspace-operator that referenced this pull request Nov 24, 2020
Reflecting devfile/api PR devfile/api#214

Cherry-picked from commit cdef6dc and
adapted slightly

Signed-off-by: David Festal <[email protected]>
amisevsk pushed a commit to devfile/devworkspace-operator that referenced this pull request Nov 27, 2020
Reflecting devfile/api PR devfile/api#214

Cherry-picked from commit cdef6dc and
adapted slightly

Signed-off-by: David Festal <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants